Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental stackmapping changes for Gen2 #1593

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

AnilMaktala
Copy link
Member

@AnilMaktala AnilMaktala commented May 31, 2024

Problem

Enabled the stack mapping property in Gen2

Issue number, if available:
aws-amplify/amplify-category-api#2550

Corresponding docs PR, if applicable:

Validation

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AnilMaktala AnilMaktala requested review from a team as code owners May 31, 2024 19:35
Copy link

changeset-bot bot commented May 31, 2024

🦋 Changeset detected

Latest commit: b168169

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/backend-data Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is missing tests.

Comment on lines +143 to +147
/**
* ExperimentalStackMapping override the assigned nested stack on a per-resource basis. Only applies to resolvers, and takes the form
* { <logicalId>: <stackName> }
*/
experimentalStackMapping?: Record<string, string>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not yet finalized a method to expose experimental APIs so please hold with this change.

@@ -239,6 +239,7 @@ class DataGenerator implements ConstructContainerEntryGenerator {
authorizationModes,
outputStorageStrategy: this.outputStorageStrategy,
functionNameMap,
stackMappings: this.props.experimentalStackMapping,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this experimental if it's mapping to construct property that is not experimental in underlying construct ?

@MarlonJD
Copy link
Contributor

MarlonJD commented Jun 1, 2024

It's just fixing important migration blocker issue #2550. stackMappings is already defined in aws-amplify/amplify-category-api repo and it was working on gen 1. We can use experimentalStackMapping as stackMapping. It's working like a charm. I hope this will merge soon. Thanks for awesome solution @AnilMaktala

Edit: It's suppressing gen 2 cli sandbox or production limit error, but in the otherside it's giving limit error on Cloudformation, I added 200 Custom mapping to 15 different stack, it's still giving limit error on Cloudformation,

We have to declare ManyToMany fields manually on Gen 2, it's creating much resources compared to gen 1, But I did some much custom stack mapping still got no luck to fix limit error.

@@ -30,6 +30,7 @@ export type DataProps = {
name?: string;
authorizationModes?: AuthorizationModes;
functions?: Record<string, ConstructFactory<AmplifyFunction>>;
experimentalStackMapping?: Record<string, string>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's review the API design internally. We may need a consolidated attribute to configure experimental features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just works like gen1. It was working property on Gen 1. Now it can be usable on Gen 2. I don't know if it's experimental or not for this reason but it's works on Cloudformation same as gen 1.

But changing stacks with gen 1 or gen 2 not effecting resource limit on Cloudformation. These are still hits the resource limit

sobolk
sobolk previously approved these changes Oct 11, 2024
@sobolk sobolk dismissed their stale review October 11, 2024 19:14

no tests. approved to early.

* feed pr base sha and ref into envs before scripts

* removing empty file
@sobolk sobolk requested a review from a team as a code owner December 3, 2024 18:31
@MarlonJD
Copy link
Contributor

MarlonJD commented Dec 4, 2024

PTAL, we need this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants